Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Floating detail panel #175

Merged
merged 20 commits into from
May 6, 2021
Merged

Floating detail panel #175

merged 20 commits into from
May 6, 2021

Conversation

bryan-brancotte
Copy link
Member

@bryan-brancotte bryan-brancotte commented May 5, 2021

Checklist

  • I indicated which issue (if any) is closed with this PR using closing keywords
  • I only changed lines related to my PR (no bulk reformatting)
  • I indicated the source and check the license if I re-use code, or I did not re-use code
  • I made my best to solve only one issue in this PR, or explain why multi had to be solved at once.

Issue

close/fix/resolve #167

details

Peek 05-05-2021 17-00

@raashika03
Copy link
Contributor

This looks good @bryan-brancotte exactly like that photoshopped one.
width of the tooltip box should be little more for better visualisation. cause a lot of space is there so.

js/tree-reusable-d3.js Outdated Show resolved Hide resolved
@bryan-brancotte
Copy link
Member Author

Peek 06-05-2021 08-56

button.btn already have display:inline-block, but div.tree-control-element doesn't, so we must set it
@raashika03
Copy link
Contributor

raashika03 commented May 6, 2021

Screenshot from 2021-05-06 15-47-39
A small remark here. Details table content flows for some screen sizes. Can you have a look over the same.
This might be helpful @bryan-brancotte

@raashika03
Copy link
Contributor

raashika03 commented May 6, 2021

I didn't understand are you planning to remove term details panel in the right side, completely from the main page by displaying on hovering.

There's no point of displaying it when it's already there on hovering.

May be only for devices which doesn't have hovering action we can let the term details panel as it's. What do you think?
Moreover this way we can reduce CPU consumption as you told here.

I'm pointing this out cause when details table is displayed on hovering I didn't look over the right-side panel even once. Didn't need that. So, it might be removed now.

And yeah those settings ideas are really great 👏

But @hmenager @matuskalas can tell better if they still want that table right-side. I'm not yet so much acquainted with the usability of edam-browser 😅

@raashika03
Copy link
Contributor

How about linking cookies for the storage of the state chosen for these three new buttons. I find that I've to change every time after refreshing. @bryan-brancotte I find this useful. 😅

This can be covered in other PR, cause current is itself a big jump in UI.

@raashika03
Copy link
Contributor

raashika03 commented May 6, 2021

Screenshot from 2021-05-06 16-51-17
@bryan-brancotte do you think this should also be fixed.
Placement of tooltip can be optimised.
PS- scroll-bar is generated but that's not useful. Scrolling isn't working here.

@bryan-brancotte
Copy link
Member Author

I didn't understand are you planning to remove term details panel in the right side, completely from the main page by displaying on hovering.

No I don't, I even don't like to have the tooltip when not in fullscreen (that is why i disabled it years ago)

There's no point of displaying it when it's already there on hovering.

agrees, but some might find usefull to hover with only community usage, (maybe ?)

How about linking cookies for the storage of the state chosen for these three new buttons. I find that I've to change every time after refreshing. @bryan-brancotte I find this useful. sweat_smile

I don't really like the tooltip, so I hesitated to save it in storage, for now no, but indeed it would be sound to have it in storage.

This can be covered in other PR, cause current is itself a big jump in UI.

Tooltip causes UI jump/glitch that is why I want to reduce its usage when not in fullscreen.

Placement of tooltip can be optimised.

Clearly, but it would be by hand, not a "simple" bootstrap attribut.

PS- scroll-bar is generated but that's not useful. Scrolling isn't working here.

It works sometime, but not all the time so not reliable

@raashika03
Copy link
Contributor

There's no point of displaying it when it's already there on hovering.

agrees, but some might find usefull to hover with only community usage, (maybe ?)

Yes it's a good feature. It'll be helpful for sure. Hovering is quick.

I don't really like the tooltip, so I hesitated to save it in storage, for now no, but indeed it would be sound to have it in storage.

Tooltip causes UI jump/glitch that is why I want to reduce its usage when not in fullscreen.

Placement of tooltip can be optimised.

Clearly, but it would be by hand, not a "simple" bootstrap attribut.

Ok @bryan-brancotte now everything sounds good to me.
Let all these for now then in future those cookies, & Ui glitch can be looked upon as per the user feedback. Good to go now 🎉

@bryan-brancotte bryan-brancotte merged commit 5e65111 into main May 6, 2021
@bryan-brancotte bryan-brancotte deleted the floating-detail-panel branch May 6, 2021 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add term detail in a floating panel in full screen
2 participants